Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Split URL Helper tests #4672

Merged
merged 2 commits into from
May 12, 2021
Merged

Split URL Helper tests #4672

merged 2 commits into from
May 12, 2021

Conversation

MGatner
Copy link
Member

@MGatner MGatner commented May 11, 2021

Description
In preparation for "Stage Two" of URI rework (targeting URL Helper) this PR splits apart the massive URL Helper test into 4 different files to make it easier to control $_SERVER manipulations and minimize overlap. These tests will have significant changes in the next PR so I wanted to split them out first to make it clear what actually is changed.

The only content updates were to testCurrentURLReturnsBasicURL() and testCurrentURLReturnsObject() which were actually bugged but didn't show it because $this->config was not injected.

Note: I realize there is significant code overlap now in the staging methods but these also are likely to change in the next PR so I have not consolidated them on purpose.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • n/a User guide updated
  • Conforms to style guide

paulbalandan
paulbalandan reviewed May 11, 2021
View reviewed changes
Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just nitpicking on my end. Can you make the name of the tests with Url in it? Like BaseTest to BaseUrlTest.

@MGatner
Copy link
Member Author

MGatner commented May 11, 2021

Just nitpicking on my end. Can you make the name of the tests with Url in it? Like BaseTest to BaseUrlTest.

Glad to! Any idea why test are flipping out?

@paulbalandan
Copy link
Member

I'm not sure. It seems the sqlite3 and intl extensions are not working.

@MGatner MGatner mentioned this pull request May 11, 2021
Merged
5 tasks
@MGatner
Copy link
Member Author

MGatner commented May 12, 2021

I'm baffled about these tests. I reran them three times and they keep having these same weird errors, even though all I did was split one file into four. Other PRs have run successful tests since so I'm not sure what to think of it.

@lonnieezell
Copy link
Member

I've seen cases where tests relied on $_SERVER or $_SESSION changes that were happening in a previous file. When that file was ran on it's own its fine, but ran in sequence and it didn't work. In those cases I was able to fix it clearing the session (or whatever) during the test setUp. Not saying that's the case here, but something is definitely going on.

The autoloader isn't getting reset somewhere, though I don't know if it matters:

'loader' => CodeIgniter\Autoloader\FileLocator Object &000000005899509a000000007780000a (
                'autoloader' => CodeIgniter\Autoloader\Autoloader Object &000000005898fd0a000000007780000a (
                    'prefixes' => Array &23 (
                        'CodeIgniter' => Array &24 (
                            0 => '/home/runner/work/CodeIgniter4/CodeIgniter4/system/'
                            1 => '/home/runner/work/CodeIgniter4/CodeIgniter4/system/'
                            2 => '/home/runner/work/CodeIgniter4/CodeIgniter4/system/'
                            // and a few hundred more of these...

@paulbalandan
Copy link
Member

I don't know if it'll make sense, but try resetting the mocked configs after each test?

@MGatner
Copy link
Member Author

MGatner commented May 12, 2021

Okay good suggestions - I will look at what changed and try mimicking.

@MGatner
Copy link
Member Author

MGatner commented May 12, 2021

That wasn't it - I didn't change the staging at all in this phase. They all reset $_SERVER = [] in tearDown() and Services::reset(true) in setUp(), same as live.

@MGatner
Copy link
Member Author

MGatner commented May 12, 2021

Try closing and reopening I guess.

@MGatner MGatner closed this May 12, 2021
@MGatner MGatner reopened this May 12, 2021
@paulbalandan
Copy link
Member

try backupGlobals enabled for all tests. I see the original test but the split tests do not except for one

@MGatner
Copy link
Member Author

MGatner commented May 12, 2021

Ooo good catch! I bet that is it, that would make total sense.

@MGatner MGatner merged commit 3509f60 into codeigniter4:develop May 12, 2021
@MGatner MGatner deleted the url-tests branch May 12, 2021 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulbalandan paulbalandan paulbalandan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MGatner @paulbalandan @lonnieezell